Skip to content

WIP: Emit warning if non-constant data columns are dropped by stat transformation. #3251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

WIP: Emit warning if non-constant data columns are dropped by stat transformation. #3251

wants to merge 2 commits into from

Conversation

clauswilke
Copy link
Member

Here is a first stab at fixing #3250, which is the silent dropping of mapped aesthetics. The code works in most cases, but there are false positives when the stat purposefully drops an aesthetic. For example, stat_boxplot() takes y and turns it into ymin, ymax, etc., and the current code doesn't know that.

One way to fix this would be to add a mechanism so that stats can declare aesthetics that are expected to be dropped, something like Stat$dropped_aesthetics, and the warning message would ignore those.

library(ggplot2)

# fill aesthetic is dropped, emit warning
ggplot(mtcars, aes(mpg, fill = am)) + geom_density(alpha = 0.5)
#> Warning: 1 aesthetic(s) dropped during statistical transformation: fill.
#> Did you forget to define a group aesthetic or to convert a numeric variable into a factor?

# fill aesthetic is not dropped because color sets up
# a grouping, no warning
ggplot(mtcars, aes(mpg, fill = am, color = factor(am))) + geom_density(alpha = 0.5)

# false positive, y aesthetic is dropped because it is converted
# to ymin, ymax, etc.
ggplot(mtcars, aes(factor(cyl), mpg)) + geom_boxplot()
#> Warning: 1 aesthetic(s) dropped during statistical transformation: y.
#> Did you forget to define a group aesthetic or to convert a numeric variable into a factor?

Created on 2019-04-19 by the reprex package (v0.2.1)

@clauswilke
Copy link
Member Author

@hadley Could you provide some high-level guidance? Should we ignore the problem or extend stats so they can suppress spurious warning messages? The latter approach would be a breaking change.

@hadley
Copy link
Member

hadley commented Apr 19, 2019

It seems reasonable, but I have no idea what existing plots this will cause to throw an error :/

@hadley hadley closed this Apr 19, 2019
@hadley hadley reopened this Apr 19, 2019
@clauswilke
Copy link
Member Author

I updated the PR so all the unit tests pass. It required one-line modifications to four stats. Seems to have a relatively modest impact and easy to fix, but undoubtedly many extension packages would have to make similar adjustments.

The more I think about it, the more I like it thought that stats have to declare any aesthetics that they drop. I've always felt that the way auxiliary aesthetics (e.g., colour) were magically carried over during the stat processing was mysterious and would break unexpectedly. This PR would create a reliable framework around this issue.

@clauswilke clauswilke mentioned this pull request Feb 6, 2020
Closed
@thomasp85
Copy link
Member

@clauswilke what are your current feelings around this - is it integrated into other internal changes we have on the map or is it fine to merge this in as is (for the release after the patch release)?

@clauswilke
Copy link
Member Author

This is a standalone modification that doesn't overlap with any other changes as far as I can see. And I keep running into the bug when teaching, so definitely something I'd like to move forward.

@thomasp85 thomasp85 added this to the ggplot2 3.4.0 milestone Apr 19, 2021
@clauswilke
Copy link
Member Author

Closing this PR in favor of #4866.

@clauswilke clauswilke closed this Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
ggplot2 3.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@clauswilke @hadley @thomasp85